[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/5] apic: add support for x2APIC mode
From: |
Igor Mammedov |
Subject: |
Re: [PATCH v2 2/5] apic: add support for x2APIC mode |
Date: |
Thu, 30 Mar 2023 10:28:50 +0200 |
On Wed, 29 Mar 2023 22:30:44 +0700
Bui Quang Minh <minhquangbui99@gmail.com> wrote:
> On 3/29/23 21:53, Bui Quang Minh wrote:
> > On 3/28/23 22:58, Bui Quang Minh wrote:
> >> On 3/27/23 23:49, David Woodhouse wrote:
> >>> On Mon, 2023-03-27 at 23:35 +0700, Bui Quang Minh wrote:
> >>>> On 3/27/23 23:22, David Woodhouse wrote:
> >>>>> On Mon, 2023-03-27 at 22:45 +0700, Bui Quang Minh wrote:
> >>>>>>
> >>>>>>> Maybe I'm misreading the patch, but to me it looks that
> >>>>>>> if (dest == 0xff) apic_get_broadcast_bitmask() bit applies even in
> >>>>>>> x2apic mode? So delivering to the APIC with physical ID 255 will be
> >>>>>>> misinterpreted as a broadcast?
> >>>>>>
> >>>>>> In case dest == 0xff the second argument to
> >>>>>> apic_get_broadcast_bitmask
> >>>>>> is set to false which means this is xAPIC broadcast
> >>>>>
> >>>>> Yeah, but it *isn't* xAPIC broadcast. It's X2APIC unicast to APIC#255.
> >>>>>
> >>>>> I think you want (although you don't have 'dev') something like this:
> >>>>>
> >>>>>
> >>>>> static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
> >>>>> uint32_t dest, uint8_t
> >>>>> dest_mode)
> >>>>> {
> >>>>> APICCommonState *apic_iter;
> >>>>> int i;
> >>>>>
> >>>>> memset(deliver_bitmask, 0x00, max_apic_words *
> >>>>> sizeof(uint32_t));
> >>>>>
> >>>>> /* x2APIC broadcast id for both physical and logical
> >>>>> (cluster) mode */
> >>>>> if (dest == 0xffffffff) {
> >>>>> apic_get_broadcast_bitmask(deliver_bitmask, true);
> >>>>> return;
> >>>>> }
> >>>>>
> >>>>> if (dest_mode == 0) {
> >>>>> apic_find_dest(deliver_bitmask, dest);
> >>>>> /* Broadcast to xAPIC mode apics */
> >>>>> - if (dest == 0xff) {
> >>>>> + if (dest == 0xff && is_x2apic_mode(dev)) {
> >>>>> apic_get_broadcast_bitmask(deliver_bitmask, false);
> >>>>> }
> >>>>> } else {
> >>>>>
> >>>>
> >>>> Hmm, the unicast case is handled in apic_find_dest function, the logic
> >>>> inside the if (dest == 0xff) is for handling the broadcast case only.
> >>>> This is because when dest == 0xff, it can be both a x2APIC unicast and
> >>>> xAPIC broadcast in case we have some CPUs that are in xAPIC and others
> >>>> are in x2APIC.
> >>>
> >>> Ah! Yes, I see it now.
> >>>
> >>> Shouldn't apic_get_broadcast_bitmask(… true) add *all* APICs to the
> >>> mask, regardless of their mode? An APIC which is still in xAPIC mode
> >>> will only look at the low 8 bits and see 0xFF which it also interprets
> >>> as broadcast? Or is that not how real hardware behaves?
> >>
> >> This is interesting. Your point looks reasonable to me but I don't
> >> know how to verify it, I'm trying to write kernel module to test it
> >> but there are just too many things running on Linux that uses
> >> interrupt so the system hangs.
> >>
> >> This raises another question: when dest == 0x102 in IPI, does the
> >> xAPIC mode CPU with APIC ID 0x2 accept the IPI? I can't see this
> >> stated clearly in the Intel SDM.
> >
> > I do some more testing on my hardware, your point is correct when dest
> > == 0xffffffff, the interrupt is delivered to all APICs regardless of
> > their mode.
>
> To be precisely, it only broadcasts to CPUs in xAPIC mode if the IPI
> destination mode is physical. In case the destination mode is logical,
> flat model/cluster model rule applies to determine if the xAPIC CPUs
> accept the IPI. Wow, this is so complicated :)
It would be nice if you could update apic kvm unit test with your
findings if it doesn't test those variants yet.
>
>
> > And when dest == 0x102 in IPI, xAPIC mode CPU with APIC ID
> > 0x2 also accepts that IPI.
>
- Re: [PATCH v2 2/5] apic: add support for x2APIC mode, (continued)
- Re: [PATCH v2 2/5] apic: add support for x2APIC mode, David Woodhouse, 2023/03/27
- Re: [PATCH v2 2/5] apic: add support for x2APIC mode, Bui Quang Minh, 2023/03/27
- Re: [PATCH v2 2/5] apic: add support for x2APIC mode, David Woodhouse, 2023/03/27
- Re: [PATCH v2 2/5] apic: add support for x2APIC mode, Bui Quang Minh, 2023/03/27
- Re: [PATCH v2 2/5] apic: add support for x2APIC mode, David Woodhouse, 2023/03/27
- Re: [PATCH v2 2/5] apic: add support for x2APIC mode, Bui Quang Minh, 2023/03/27
- Re: [PATCH v2 2/5] apic: add support for x2APIC mode, David Woodhouse, 2023/03/27
- Re: [PATCH v2 2/5] apic: add support for x2APIC mode, Bui Quang Minh, 2023/03/28
- Re: [PATCH v2 2/5] apic: add support for x2APIC mode, Bui Quang Minh, 2023/03/29
- Re: [PATCH v2 2/5] apic: add support for x2APIC mode, Bui Quang Minh, 2023/03/29
- Re: [PATCH v2 2/5] apic: add support for x2APIC mode,
Igor Mammedov <=
[PATCH v2 3/5] apic, i386/tcg: add x2apic transitions, Bui Quang Minh, 2023/03/26
[PATCH v2 4/5] intel_iommu: allow Extended Interrupt Mode when using userspace APIC, Bui Quang Minh, 2023/03/26
[PATCH v2 5/5] amd_iommu: report x2APIC support to the operating system, Bui Quang Minh, 2023/03/26