Re: [PATCH v5 3/4] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server.

[Yu Zhang <yu.c.zhang@linux.intel.com>]

On 8/8/2016 11:40 PM, Jan Beulich wrote:
>>>> On 12.07.16 at 11:02, <yu.c.zhang@linux.intel.com> wrote:
>> @@ -178,8 +179,34 @@ static int hvmemul_do_io(
>>           break;
>>       case X86EMUL_UNHANDLEABLE:
>>       {
>> -        struct hvm_ioreq_server *s =
>> -            hvm_select_ioreq_server(curr->domain, &p);
>> +        struct hvm_ioreq_server *s;
>> +
>> +        if ( is_mmio )
>> +        {
>> +            unsigned long gmfn = paddr_to_pfn(addr);
>> +            p2m_type_t p2mt;
>> +
>> +            (void) get_gfn_query_unlocked(currd, gmfn, &p2mt);
>> +
>> +            if ( p2mt == p2m_ioreq_server )
>> +            {
>> +                unsigned int flags;
>> +
>> +                if ( dir != IOREQ_WRITE )
>> +                    s = NULL;
>> +                else
>> +                {
>> +                    s = p2m_get_ioreq_server(currd, &flags);
>> +
>> +                    if ( !(flags & P2M_IOREQ_HANDLE_WRITE_ACCESS) )
>> +                        s = NULL;
>> +                }
>> +            }
>> +            else
>> +                s = hvm_select_ioreq_server(currd, &p);
>> +        }
>> +        else
>> +            s = hvm_select_ioreq_server(currd, &p);
> Wouldn't it both be more natural and make the logic even easier
> to follow if s got set to NULL up front, all the "else"-s dropped,
> and a simple
>
>          if ( !s )
>              s = hvm_select_ioreq_server(currd, &p);
>
> be done in the end?

Thanks, Jan. I'll have a try.

>> @@ -5447,6 +5452,21 @@ static int hvmop_set_mem_type(
>>       if ( !is_hvm_domain(d) )
>>           goto out;
>>   
>> +    if ( a.hvmmem_type == HVMMEM_ioreq_server )
>> +    {
>> +        unsigned int flags;
>> +        struct hvm_ioreq_server *s;
>> +
>> +        /* HVMMEM_ioreq_server is only supported for HAP enabled hvm. */
>> +        if ( !hap_enabled(d) )
>> +            goto out;
>> +
>> +        /* Do not change to HVMMEM_ioreq_server if no ioreq server mapped. */
>> +        s = p2m_get_ioreq_server(d, &flags);
>> +        if ( s == NULL )
>> +            goto out;
> Either drop s as an intermediate variable altogether (preferred), or
> constify it properly.

Got it.

>> +int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
>> +                                     uint32_t type, uint32_t flags)
>> +{
>> +    struct hvm_ioreq_server *s;
>> +    int rc;
>> +
>> +    /* For now, only HVMMEM_ioreq_server is supported. */
>> +    if ( type != HVMMEM_ioreq_server )
>> +        return -EINVAL;
>> +
>> +    /* For now, only write emulation is supported. */
>> +    if ( flags & ~(XEN_HVMOP_IOREQ_MEM_ACCESS_WRITE) )
>> +        return -EINVAL;
>> +
>> +    spin_lock(&d->arch.hvm_domain.ioreq_server.lock);
> This lock did get converted to a recursive one a little while back.

Oh. I see, should use the spin_lock_recursive/spin_unlock_recursive 
pair. Thanks.

>> +    rc = -ENOENT;
>> +    list_for_each_entry ( s,
>> +                          &d->arch.hvm_domain.ioreq_server.list,
>> +                          list_entry )
>> +    {
>> +        if ( s == d->arch.hvm_domain.default_ioreq_server )
>> +            continue;
>> +
>> +        if ( s->id == id )
>> +        {
>> +            rc = p2m_set_ioreq_server(d, flags, s);
>> +            if ( rc == 0 )
>> +                dprintk(XENLOG_DEBUG, "%u %s type HVMMEM_ioreq_server.\n",
>> +                         s->id, (flags != 0) ? "mapped to" : "unmapped from");
> Is this really useful?

Sorry, are you referring to this debug message?
I believe it helps, especially when multiple ioreq servers are 
claiming/disclaiming their ownership of
the HVMMEM_ioreq_server. :)

>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -132,6 +132,13 @@ static void ept_p2m_type_to_flags(struct p2m_domain *p2m, ept_entry_t *entry,
>>               entry->r = entry->w = entry->x = 1;
>>               entry->a = entry->d = !!cpu_has_vmx_ept_ad;
>>               break;
>> +        case p2m_ioreq_server:
>> +            entry->r = 1;
>> +            entry->w = !(p2m->ioreq.flags & P2M_IOREQ_HANDLE_WRITE_ACCESS);
>> +            entry->x = 0;
>> +            entry->a = !!cpu_has_vmx_ept_ad;
>> +            entry->d = entry->w && cpu_has_vmx_ept_ad;
> For self-consistency, could this become
>
>              entry->d = entry->w && entry->a;
>
> ?

Yep. Thanks. :)

>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -3225,8 +3225,7 @@ static int sh_page_fault(struct vcpu *v,
>>       }
>>   
>>       /* Need to hand off device-model MMIO to the device model */
>> -    if ( p2mt == p2m_mmio_dm
>> -         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
>> +    if ( p2mt == p2m_mmio_dm )
> Could you remind me again what the code being removed here gets
> replaced by, or why it doesn't need any replacement?

  HVMMEM_ioreq_server is now only supported for HAP enabled hvm, so 
shadow code
does not need to handle this p2m type.

>> @@ -336,6 +336,23 @@ struct p2m_domain {
>>           struct ept_data ept;
>>           /* NPT-equivalent structure could be added here. */
>>       };
>> +
>> +    struct {
>> +        spinlock_t lock;
>> +        /*
>> +         * ioreq server who's responsible for the emulation of
>> +         * gfns with specific p2m type(for now, p2m_ioreq_server).
>> +         */
>> +        struct hvm_ioreq_server *server;
>> +        /*
>> +         * flags specifies whether read, write or both operations
>> +         * are to be emulated by an ioreq server.
>> +         */
>> +        unsigned int flags;
>> +
>> +#define P2M_IOREQ_HANDLE_WRITE_ACCESS XEN_HVMOP_IOREQ_MEM_ACCESS_WRITE
>> +#define P2M_IOREQ_HANDLE_READ_ACCESS  XEN_HVMOP_IOREQ_MEM_ACCESS_READ
> I think I did say so on a previous iteration already: I can't see the
> value of these two defines, or in fact I can see these being actively
> dangerous: The rest of your patch assume that each pair shares
> their values (as there's no translation between them, but also no
> BUILD_BUG_ON() ensuring they're identical).

Oh. I thought it was a coding style issue we were discussing - 
previously,  there was a
#define using the <underscore><uppercase> pattern, which should be 
deprecated.

For the P2M_IOREQ_HANDLE_WRITE/READ_ACCESS definition, I agree they can 
be removed.

>
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -89,7 +89,9 @@ typedef enum {
>>       HVMMEM_unused,             /* Placeholder; setting memory to this type
>>                                     will fail for code after 4.7.0 */
>>   #endif
>> -    HVMMEM_ioreq_server
>> +    HVMMEM_ioreq_server        /* Memory type claimed by an ioreq server; type
>> +                                  changes to this value are only allowed after
>> +                                  an ioreq server has claimed its ownership. */
> Wouldn't it be worth also noting in the comment that only changes
> to/from rw are permitted?
>

OK. Will add this comment in next version.

Yu

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel