>>> On 06.09.16 at 10:03, <paul.durr...@citrix.com> wrote: >> -----Original Message----- >> From: Jan Beulich [mailto:jbeul...@suse.com] >> Sent: 06 September 2016 08:58 >> To: George Dunlap <george.dun...@citrix.com>; Yu Zhang >> <yu.c.zh...@linux.intel.com> >> Cc: Andrew Cooper <andrew.coop...@citrix.com>; Paul Durrant >> <paul.durr...@citrix.com>; George Dunlap <george.dun...@citrix.com>; >> JunNakajima <jun.nakaj...@intel.com>; Kevin Tian <kevin.t...@intel.com>; >> zhiyuan...@intel.com; xen-devel@lists.xen.org; Tim (Xen.org) >> <t...@xen.org> >> Subject: Re: [PATCH v6 1/4] x86/ioreq server: Add HVMOP to map guest ram >> with p2m_ioreq_server to an ioreq server. >> >> >>> On 05.09.16 at 19:20, <george.dun...@citrix.com> wrote: >> > On 05/09/16 14:31, Jan Beulich wrote: >> >>>>> On 02.09.16 at 12:47, <yu.c.zh...@linux.intel.com> 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. >> > > My understanding is that we want accesses that make it here for pages that > are not of type 'ioreq_server' to result in MMIO emulation (i.e. they hit an > emulator's requested ranges, or the access is completed as unclaimed MMIO by > Xen). Accesses that make it here because the page *is* of type 'ioreq server' > should be sent to the emulator that has claimed the type and, if no emulator > does currently have a claim to the type, be handled as if the access was to > r/w RAM.
Makes sense, but it doesn't look like the code above does that, as - keeping s to be NULL means ignoring the access - finding a server by some means would imply handling the access as I/O instead of RAM. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
