[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 16:50, Rahul Singh wrote: >> On 9 Mar 2022, at 10:17 am, Jan Beulich <jbeulich@xxxxxxxx> wrote: >> On 09.03.2022 11:08, Rahul Singh wrote: >>>> 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? Yes, but ... > 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. ... with Roger agreeing with this plan, that other alternative is likely dead now. Provided other stuff which isn't obviously arch- specific remains in common code. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |