From: Yan Zhao <yan.y.zhao@intel.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "Paolo Bonzini" <pbonzini@redhat.com>,
"Alex Williamson" <alex.williamson@redhat.com>,
"Zeng, Xin" <xin.zeng@intel.com>,
"QEMU Developers" <qemu-devel@nongnu.org>,
"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [PATCH v6 1/3] memory: drop guest writes to read-only ram device regions
Date: Thu, 30 Apr 2020 06:11:09 -0400 [thread overview]
Message-ID: <20200430101108.GR12879@joy-OptiPlex-7040> (raw)
In-Reply-To: <CAFEAcA-8NH_4ZV0J9urBZdQWmqOe-Nyy4y2gLAjTJ08MpfFY0g@mail.gmail.com>
On Thu, Apr 30, 2020 at 05:40:25PM +0800, Peter Maydell wrote:
> On Thu, 30 Apr 2020 at 09:20, Yan Zhao <yan.y.zhao@intel.com> wrote:
> >
> > for ram device regions, drop guest writes if the region is read-only.
> >
> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> > Signed-off-by: Xin Zeng <xin.zeng@intel.com>
> > ---
> > memory.c | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/memory.c b/memory.c
> > index 601b749906..a1bba985b9 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -34,6 +34,7 @@
> > #include "sysemu/accel.h"
> > #include "hw/boards.h"
> > #include "migration/vmstate.h"
> > +#include "qemu/log.h"
> >
> > //#define DEBUG_UNASSIGNED
> >
> > @@ -1307,12 +1308,19 @@ static uint64_t memory_region_ram_device_read(void *opaque,
> > return data;
> > }
> >
> > -static void memory_region_ram_device_write(void *opaque, hwaddr addr,
> > - uint64_t data, unsigned size)
> > +static MemTxResult memory_region_ram_device_write(void *opaque, hwaddr addr,
> > + uint64_t data, unsigned size,
> > + MemTxAttrs attrs)
> > {
> > MemoryRegion *mr = opaque;
> >
> > trace_memory_region_ram_device_write(get_cpu_index(), mr, addr, data, size);
> > + if (mr->readonly) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "Invalid write to read-only ram device region addr 0x%"
> > + HWADDR_PRIx" size %u\n", addr, size);
> > + return MEMTX_ERROR;
> > + }
>
> This does not "drop" a write to a r/o region -- it causes it to generate
> whatever the guest architecture's equivalent of a bus error is (eg data
> abort on Arm).
>
hmm, I'm not sure. so your expectation is silently dropping guest writes
without any bus error, right?
> More generally, this change seems a bit odd: currently we do not
> check the mr->readonly flag here, but in general guests don't get
> to write to ROM areas. Where is that check currently done, and
it's not a ROM, but a ram region backed by a device. we wish this region
to be read-only sometimes, in order to implement some useful features.
It can be a virtual BAR region in a virtual mdev device.
> should the vfio case you're trying to fix do its check in whatever
> the equivalent of that place is? Alternatively, if we want to make
> memory_region_ram_device_write() do the check, does that mean we
> now have unnecessary checks elsewhere.
currently, vfio implements the BAR regions in two types:
1. non-mmap'd, meaning this region will not be added into kvm memory
slots, and whenever guest accesses it, it will be trapped into a host
handler. we do the read-only check in patch 2 of this series.
2. mmap'd, meaning this region will be added into kvm memory slots, and
guest could access it without any hypervisor intervening.
so without patch 3 in the series, there's no write protection to guest
writes.
after setting this mmap'd region to read-only in patch 3, the
corresponding memory slot in kvm is set to read-only, so only guest
writes would be trapped into host, i.e. into the
memory_region_ram_device_write(). guest reads is still within the guest
without hypervisor intervening.
>
> My guess is that memory_region_ram_device_write() isn't the
> right place to check for read-only-ness, because it only applies
> to RAM-backed MRs, not to any other kind of MR which might equally
> be readonly.
>
there might be other MRs that require checking of read-only-ness.
but their handlers have the right to be called to know it has happened,
and they might want to do some special handling of it. That's why I did
not put the check in general dispatcher.
Thanks
Yan
next prev parent reply other threads:[~2020-04-30 10:35 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-30 8:07 [PATCH v6 0/3] drop writes to read-only ram device & vfio regions Yan Zhao
2020-04-30 8:09 ` [PATCH v6 1/3] memory: drop guest writes to read-only ram device regions Yan Zhao
2020-04-30 9:40 ` Peter Maydell
2020-04-30 10:11 ` Yan Zhao [this message]
2020-05-21 14:38 ` Paolo Bonzini
2020-05-25 1:18 ` Yan Zhao
2020-05-25 10:20 ` Paolo Bonzini
2020-05-25 10:54 ` Philippe Mathieu-Daudé
2020-05-25 11:04 ` Paolo Bonzini
2020-05-26 2:11 ` Yan Zhao
2020-05-26 9:14 ` Peter Maydell
2020-05-26 9:26 ` Peter Maydell
2020-05-28 4:35 ` Yan Zhao
2020-05-28 5:10 ` Paolo Bonzini
2020-05-28 6:15 ` Yan Zhao
2020-04-30 8:13 ` [PATCH v6 2/3] hw/vfio: drop guest writes to ro regions Yan Zhao
2020-04-30 8:13 ` [PATCH v6 3/3] hw/vfio: let read-only flag take effect for mmap'd regions Yan Zhao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200430101108.GR12879@joy-OptiPlex-7040 \
--to=yan.y.zhao@intel.com \
--cc=alex.williamson@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=philmd@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=xin.zeng@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).