Re: [Xen-devel] [PATCH v4 2/9] x86/mmcfg: add handlers for the PVH Dom0 MMCFG areas

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> 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