Re: [PATCH 1/2] x86/shadow: sanitize iommu_snoop usage

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 16/01/2023 8:56 am, Jan Beulich wrote:
> On 13.01.2023 12:55, Andrew Cooper wrote:
>> On 13/01/2023 8:47 am, Jan Beulich wrote:
>>> In _sh_propagate() I'm further puzzled: The iomem_access_permitted()
>>> certainly suggests very bad things could happen if it returned false
>>> (i.e. in the implicit "else" case). The assumption looks to be that no
>>> bad "target_mfn" can make it there. But overall things might end up
>>> looking more sane (and being cheaper) when simply using "mmio_mfn"
>>> instead.
>> That entire block looks suspect.  For one, I can't see why the ASSERT()
>> is correct; we have literally just (conditionally) added CACHE_ATTR to
>> pass_thru_flags and pulled everything across from gflags into sflags.
> That is for !shadow_mode_refcounts() domains, i.e. PV, whereas the
> outermost conditional here limits things to HVM. Using different
> predicates of course obfuscates this some, but bringing those two
> closer together (perhaps even merging them) didn't look reasonable
> to do right here.

Ah, that bit.  Also further obfuscated by partial nested !'s.

I doubt Shadow has seen anything beyond token testing in combination
with PCI Passthrough.  It certainly saw no testing under XenServer.

>> It can also half its number of external calls by rearranging the if/else
>> chain and making better use of the type variable.
> I did actually spend quite a bit of time to see whether I could figure
> a valid way of re-arranging the order, but in the end for every
> transformation I found a reason why it wouldn't be valid. So I'm
> curious what valid simplification(s) you see.

Well, the first two calls calls to pat_type_2_pte_flags() can be merged
quite easily, but I was also thinking in terms of future where coherency
handling was working in a more sane way.

>>> @@ -571,7 +571,7 @@ _sh_propagate(struct vcpu *v,
>>>                              gfn_to_paddr(target_gfn),
>>>                              mfn_to_maddr(target_mfn),
>>>                              X86_MT_UC);
>>> -                else if ( iommu_snoop )
>>> +                else if ( is_iommu_enabled(d) && iommu_snoop )
>>>                      sflags |= pat_type_2_pte_flags(X86_MT_WB);
>> Hmm...  This is still one reasonably expensive nop; the PTE Flags for WB
>> are 0.
> Right, but besides being unrelated to the patch (there's a following
> "else", so the condition cannot be purged altogether) I would wonder
> if we really want to bake in more PAT layout <-> PTE dependencies.

I'm not advocating for more assumption about PAT <-> PTE layout, but it
would be nice if the NOPs were actually NOPs.

I submitted a patch which makes pat_type_2_pte_flags() marginally less
expensive, but there's still massive savings to be made here.  Because
XEN's PAT is compile time constant, this inverse can be too.

>
>>> --- a/xen/drivers/passthrough/x86/iommu.c
>>> +++ b/xen/drivers/passthrough/x86/iommu.c
>>> @@ -56,6 +56,13 @@ void __init acpi_iommu_init(void)
>>>      if ( !acpi_disabled )
>>>      {
>>>          ret = acpi_dmar_init();
>>> +
>>> +#ifndef iommu_snoop
>>> +        /* A command line override for snoop control affects VT-d only. */
>>> +        if ( ret )
>>> +            iommu_snoop = true;
>>> +#endif
>> I really don't think this is a good idea.  If nothing else, you're
>> reinforcing the notion that this logic is somehow acceptable.
>>
>> If instead the comment read something like:
>>
>> /* This logic is pretty bogus, but necessary for now.  iommu_snoop as a
>> control is only wired up for VT-d (which may be conditionally compiled
>> out), and while AMD can control coherency, Xen forces coherent accesses
>> unilaterally so iommu_snoop needs to report true on all AMD systems for
>> logic elsewhere in Xen to behave correctly. */
> I've extended the comment to this:
>
>         /*
>          * As long as there's no per-domain snoop control, and as long as on
>          * AMD we uniformly force coherent accesses, a possible command line
>          * override should affect VT-d only.
>          */

Better.  I suppose my displeasure of this can live on list...

~Andrew