Re: [Xen-devel] [PATCH v6 1/4] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.09.16 at 19:20, <george.dunlap@xxxxxxxxxx> wrote:
> On 05/09/16 14:31, Jan Beulich wrote:
>>>>> On 02.09.16 at 12:47, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
>>> @@ -178,8 +179,27 @@ 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 = NULL;
>>> +        p2m_type_t p2mt = p2m_invalid;
>>> +
>>> +        if ( is_mmio )
>>> +        {
>>> +            unsigned long gmfn = paddr_to_pfn(addr);
>>> +
>>> +            (void) get_gfn_query_unlocked(currd, gmfn, &p2mt);
>>> +
>>> +            if ( p2mt == p2m_ioreq_server && dir == IOREQ_WRITE )
>>> +            {
>>> +                unsigned int flags;
>>> +
>>> +                s = p2m_get_ioreq_server(currd, &flags);
>>> +                if ( !(flags & XEN_HVMOP_IOREQ_MEM_ACCESS_WRITE) )
>>> +                    s = NULL;
>>> +            }
>>> +        }
>>> +
>>> +        if ( !s && p2mt != p2m_ioreq_server )
>>> +            s = hvm_select_ioreq_server(currd, &p);
>> 
>> What I recall is that we had agreed on p2m_ioreq_server pages
>> to be treated as ordinary RAM ones as long as no server can be
>> found. The type check here contradicts that. Is there a reason?
> 
> I think it must be a confusion as to what "treated like ordinary RAM
> ones" means.  p2m_ram_rw types that gets here will go through
> hvm_select_ioreq_server(), and (therefore) potentially be treated as
> MMIO accesses, which is not how "ordinary RAM" would behave.  If what
> you meant was that you want p2m_ioreq_server to behave like p2m_ram_rw
> (and be treated as MMIO if it matches an iorange) then yes.  If what you
> want is for p2m_ioreq_server to actually act like RAM, then probably
> something more needs to happen here.

Well, all I'm questioning is the special casing of p2m_ioreq_server in
the if(). That's imo orthogonal to p2m_ram_rw pages not being
supposed to make it here (hence the is_mmio check in the earlier if()
also looks questionable). Perhaps it would already help if there was
a comment explaining what the exact intended behavior here is.

Jan


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