[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 2/9] x86/mmcfg: add handlers for the PVH Dom0 MMCFG areas
>>> Roger Pau Monné <roger.pau@xxxxxxxxxx> 07/14/17 6:34 PM >>> >On Thu, Jul 13, 2017 at 02:15:26PM -0600, Jan Beulich wrote: >> >>> Roger Pau Monne <roger.pau@xxxxxxxxxx> 06/30/17 5:02 PM >>> >> > @@ -1041,6 +1043,24 @@ static int __init pvh_setup_acpi(struct domain *d, >> > paddr_t start_info) >> > return 0; >> > } >> > >> > +int __init pvh_setup_mmcfg(struct domain *d) >> >> Didn't I point out that __init van't be correct here, and instead this >> needs to be __hwdom_init? I can see that the only current caller is >> __init, but that merely suggests there is a second call missing. > >Mostly likely, and I failed to update it. > >AFAIK it's not possible to build a late PVH hwdom (or I don't see >how), so I guess that missing call should be added if we ever support >that. Why would a late hwdom not be able to be PVH? All depends on whether the tool (stack) to build it (in domain 0) is capable of that. >> > --- a/xen/arch/x86/hvm/io.c >> > +++ b/xen/arch/x86/hvm/io.c >> > @@ -261,11 +261,11 @@ void register_g2m_portio_handler(struct domain *d) >> > static int vpci_access_check(unsigned int reg, unsigned int len) >> > { >> > /* Check access size. */ >> > - if ( len != 1 && len != 2 && len != 4 ) >> > + if ( len != 1 && len != 2 && len != 4 && len != 8 ) >> > return -EINVAL; >> > >> > - /* Check if access crosses a double-word boundary. */ >> > - if ( (reg & 3) + len > 4 ) >> > + /* Check if access crosses a double-word boundary or it's not >> > aligned. */ >> > + if ( (len <= 4 && (reg & 3) + len > 4) || (len == 8 && (reg & 3) != >> > 0) ) >> > return -EINVAL; >> >> For one I suppose you mean "& 7" in the 8-byte case. > >I cannot find anything in the PCIe 3.1A specification that says that >8B accesses should be aligned. AFAICT it only mentions that accesses >should not cross double-word (4B) boundaries, because it's not >mandatory for the root complex to support such accesses. Hmm, ugly. I'd be particularly concerned about an 8-byte access crossing the standard/extended config space boundary, or one crossing the boundary between two devices (or worse between a device and a hole). I'd suggest to be conservative for now and require full alignment. >> And then I don't >> understand why you permit mis-aligned 2-byte writes, but not mis-aligned >> 4-byte ones as long as they fall withing a quad-word. Any such asymmetry >> needs at least a comment. > >IIRC reading soemthing like that on the Mindshare PCI book, but I >don't have it at hand. Will check on Monday. Anyway, I cannot seem to >find any specific set of restrictions in the PCI/PCIe specifications, >apart from the one that accesses should not cross a double-word >boundary. > >I'm fine with only allowing accesses aligned to their respective >sizes, but I think I should add a comment somewhere regarding where >this has been picked from. Do you have any references from the >AMD/Intel SDMs maybe? No, I'm sorry. >> > +static int vpci_mmcfg_accept(struct vcpu *v, unsigned long addr) >> > +{ >> > + struct domain *d = v->domain; >> > + bool found; >> > + >> > + vpci_lock(d); >> > + found = vpci_mmcfg_find(d, addr); >> > + vpci_unlock(d); >> >> The latest here I wonder whether the lock wouldn't better be an r/w one. > >TBH, my first implementation was using a rw lock, but then I though it >was not worth it and switched to a spinlock. I Don't mind making it a >rw lock, but then the argument passed to the read handlers should be >constified for safety IMHO. Which of the arguments? >Also note that due to the usage of the pcidevs lock whether this is rw >or a spinlock doesn't make much of a difference. True. >> > +static int vpci_mmcfg_read(struct vcpu *v, unsigned long addr, >> > + unsigned int len, unsigned long *data) >> >> uint64_t * (to be 32-bit compatible) > >Will this work properly on 32bit builds? 32-bit builds of what? For 32-bit ARM this is only a future (if ever) consideration - r>hvm_mmio_{read/write}_t types expect a unsigned long, not a >uint64_t. I'm confused about how this worked before with a 32bit >hypervisor and a 64bit guest, how where movq handled? I think all this abstraction postdates removal of x86-32 builds. As to MOVQ - if you think about the MMX/SSE variants of it, 32-bit would have split the access just like 64-bit splits e.g. MOVDQA. >> > + pcidevs_unlock(); >> > + vpci_unlock(d); >> >> Question on lock order (should have gone into the patch 1 reply already, >> but I had thought of this only after sending): Is it really a good idea >> to nest this way? > >I saw no other way to make sure the pdev is not removed while poking >at it. As long as all of this is Dom0-only, I don't think that's a major concern. As said elsewhere, we don't consistently lock against device removal anyway, and we should rather use refcounting to deal with this. >> The pcidevs lock is covering quite large regions at >> times, so the risk of a lock order violation seems non-negligible even >> if there may be none right now. Futhermore the new uses of the pcidevs >> lock you introduce would seem to make it quite desirable to make that >> one an r/w one too. Otoh that's a recursive one, so it'll be non-trivial >> to convert ... > >I can try, but as you say doesn't seem trivial at all. So perhaps better to continue assuming a well behaved Dom0 here for now? Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |