Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Mar 10, 2022 at 11:48:15AM +0000, Rahul Singh wrote:
> Hello Roger,
> 
> > On 9 Mar 2022, at 4:06 pm, Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> > 
> > On Wed, Mar 09, 2022 at 03:50:12PM +0000, Rahul Singh wrote:
> >> Hi Jan,
> >> 
> >>> On 9 Mar 2022, at 10:17 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>> 
> >>> On 09.03.2022 11:08, Rahul Singh wrote:
> >>>> Hi Jan,
> >>>> 
> >>>>> On 4 Mar 2022, at 7:23 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>>> 
> >>>>> On 03.03.2022 17:31, Rahul Singh wrote:
> >>>>>>> On 1 Mar 2022, at 1:55 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>>>>> On 01.03.2022 14:34, Rahul Singh wrote:
> >>>>>>>>> On 24 Feb 2022, at 2:57 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>>>>>>> On 15.02.2022 16:25, Rahul Singh wrote:
> >>>>>>>>>> --- a/xen/arch/x86/hvm/vmsi.c
> >>>>>>>>>> +++ b/xen/arch/x86/hvm/vmsi.c
> >>>>>>>>>> @@ -925,4 +925,106 @@ int vpci_msix_arch_print(const struct 
> >>>>>>>>>> vpci_msix *msix)
> >>>>>>>>>> 
> >>>>>>>>>> return 0;
> >>>>>>>>>> }
> >>>>>>>>>> +
> >>>>>>>>>> +int vpci_make_msix_hole(const struct pci_dev *pdev)
> >>>>>>>>>> +{
> >>>>>>>>>> +    struct domain *d = pdev->domain;
> >>>>>>>>>> +    unsigned int i;
> >>>>>>>>>> +
> >>>>>>>>>> +    if ( !pdev->vpci->msix )
> >>>>>>>>>> +        return 0;
> >>>>>>>>>> +
> >>>>>>>>>> +    /* Make sure there's a hole for the MSIX table/PBA in the 
> >>>>>>>>>> p2m. */
> >>>>>>>>>> +    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->msix->tables); i++ )
> >>>>>>>>>> +    {
> >>>>>>>>>> +        unsigned long start = 
> >>>>>>>>>> PFN_DOWN(vmsix_table_addr(pdev->vpci, i));
> >>>>>>>>>> +        unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, 
> >>>>>>>>>> i) +
> >>>>>>>>>> +                                     vmsix_table_size(pdev->vpci, 
> >>>>>>>>>> i) - 1);
> >>>>>>>>>> +
> >>>>>>>>>> +        for ( ; start <= end; start++ )
> >>>>>>>>>> +        {
> >>>>>>>>>> +            p2m_type_t t;
> >>>>>>>>>> +            mfn_t mfn = get_gfn_query(d, start, &t);
> >>>>>>>>>> +
> >>>>>>>>>> +            switch ( t )
> >>>>>>>>>> +            {
> >>>>>>>>>> +            case p2m_mmio_dm:
> >>>>>>>>>> +            case p2m_invalid:
> >>>>>>>>>> +                break;
> >>>>>>>>>> +            case p2m_mmio_direct:
> >>>>>>>>>> +                if ( mfn_x(mfn) == start )
> >>>>>>>>>> +                {
> >>>>>>>>>> +                    clear_identity_p2m_entry(d, start);
> >>>>>>>>>> +                    break;
> >>>>>>>>>> +                }
> >>>>>>>>>> +                /* fallthrough. */
> >>>>>>>>>> +            default:
> >>>>>>>>>> +                put_gfn(d, start);
> >>>>>>>>>> +                gprintk(XENLOG_WARNING,
> >>>>>>>>>> +                        "%pp: existing mapping (mfn: %" PRI_mfn
> >>>>>>>>>> +                        "type: %d) at %#lx clobbers MSIX MMIO 
> >>>>>>>>>> area\n",
> >>>>>>>>>> +                        &pdev->sbdf, mfn_x(mfn), t, start);
> >>>>>>>>>> +                return -EEXIST;
> >>>>>>>>>> +            }
> >>>>>>>>>> +            put_gfn(d, start);
> >>>>>>>>>> +        }
> >>>>>>>>>> +    }
> >>>>>>>>>> +
> >>>>>>>>>> +    return 0;
> >>>>>>>>>> +}
> >>>>>>>>> 
> >>>>>>>>> ... nothing in this function looks to be x86-specific, except maybe
> >>>>>>>>> functions like clear_identity_p2m_entry() may not currently be 
> >>>>>>>>> available
> >>>>>>>>> on Arm. But this doesn't make the code x86-specific.
> >>>>>>>> 
> >>>>>>>> I will maybe be wrong but what I understand from the code is that 
> >>>>>>>> for x86 
> >>>>>>>> if there is no p2m entries setup for the region, accesses to them 
> >>>>>>>> will be trapped 
> >>>>>>>> into the hypervisor and can be handled by specific MMIO handler.
> >>>>>>>> 
> >>>>>>>> But for ARM when we are registering the MMIO handler we have to 
> >>>>>>>> provide 
> >>>>>>>> the GPA also for the MMIO handler. 
> >>>>>>> 
> >>>>>>> Question is: Is this just an effect resulting from different 
> >>>>>>> implementation,
> >>>>>>> or an inherent requirement? In the former case, harmonizing things 
> >>>>>>> may be an
> >>>>>>> alternative option.
> >>>>>> 
> >>>>>> This is an inherent requirement to provide a GPA when registering the 
> >>>>>> MMIO handler.
> >>>>> 
> >>>>> So you first say yes to my "inherent" question, but then ...
> >>>>> 
> >>>>>> For x86 msix mmio handlers is registered in init_msix(..) function as 
> >>>>>> there is no requirement
> >>>>>> on x86 to provide GPA when registering the handler. Later point of 
> >>>>>> time when BARs are configured
> >>>>>> and memory decoding bit is enabled vpci_make_msix_hole() will clear 
> >>>>>> the identity mapping for msix
> >>>>>> base table address so that access to msix tables will be trapped.
> >>>>>> 
> >>>>>> On ARM we need to provide GPA to register the mmio handler and MSIX 
> >>>>>> table base
> >>>>>> address is not valid when init_msix() is called as BAR will be 
> >>>>>> configured later point in time.
> >>>>>> Therefore on ARM mmio handler will be registered in function 
> >>>>>> vpci_make_msix_hole() when
> >>>>>> memory decoding bit is enabled.
> >>>>> 
> >>>>> ... you explain it's an implementation detail. I'm inclined to
> >>>>> suggest that x86 also pass the GPA where possible. Handler lookup
> >>>>> really would benefit from not needing to iterate over all registered
> >>>>> handlers, until one claims the access. The optimization part of this
> >>>>> of course doesn't need to be done right here, but harmonizing
> >>>>> register_mmio_handler() between both architectures would seem to be
> >>>>> a reasonable prereq step.
> >>>> 
> >>>> I agree with you that if we modify the register_mmio_handler() for x86 
> >>>> to pass GPA
> >>>> we can have the common code for x86 and ARM and also we can optimize the 
> >>>> MMIO
> >>>> trap handling for x86.
> >>>> 
> >>>> What I understand from the code is that modifying the 
> >>>> register_mmio_handler() for
> >>>> x86 to pass GPA requires a lot of effort and testing.
> >>>> 
> >>>> Unfortunately, I have another high priority task that I have to complete 
> >>>> I don’t have time
> >>>> to optimise the register_mmio_handler() for x86 at this time.
> >>> 
> >>> Actually making use of the parameter is nothing I would expect you to
> >>> do. But is just adding the extra parameter similarly out of scope for
> >>> you?
> >>> 
> >> 
> >> If I understand correctly you are asking to make register_mmio_handler() 
> >> declaration
> >> same for x86 and ARM. You are not asking to modify the x86 MMIO handler 
> >> logic to
> >> use GPA to find the handler?
> >> 
> >> As Roger also mentioned that vpci_make_msix_hole() is required only for 
> >> x86 to clear
> >> the identity mapping. If we make the vpci_make_msix_hole() arch-specific 
> >> there is no
> >> need to modify the parameter for register_mmio_handler(), as for x86 and 
> >> ARM
> >> register_mmio_handler() will be called in different places.
> >> 
> >> For x86 register_mmio_handler() will be called in init_msix() whereas for 
> >> ARM
> >> register_mmio_handler() will be called  in vpci_make_msix_hole(). In this 
> >> case we
> >> have to move the call to register_mmio_handler() also to arch-specific 
> >> files. If we move
> >> the register_mmio_handler()  to arch specific file there is no need to 
> >> make the
> >> function common.
> > 
> > So then for Arm you will need something akin to
> > unregister_mmio_handler so the handler can be removed when memory
> > decoding is disabled?
> > 
> > Or else you would keep adding new handlers every time the guest
> > enables memory decoding for the device without having removed the
> > stale ones?
> 
> Yes, when I will send the patches for ARM I will take care of this not to 
> register the handler 
> again if the memory decoding bit is changed. Before registering the handler 
> will check 
> if the handler is already for GPA if it is already registered no need to 
> register.

I think it might be helpful to post the Arm bits together with the
moving of the x86 ones. It's way easier to see why you need to make
certain things arch-specific if you also provide the Arm
implementation at the same time. Or else it's just code movement that
might need to be redone when Arm support is introduced if we deem that
certain parts could be unified.

Thanks, Roger.