[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 1/3] xen/vpci: msix: move x86 specific code to x86 file
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? Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |