| Message ID | 20210805152804.100333-12-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: skey related fixes, cleanups, and memory device preparations | expand |
On 05/08/2021 17.28, David Hildenbrand wrote: > Let's validate the given address and report a proper error in case it's > not. All call paths now properly check the validity of the given GFN. > Remove the TODO. > > The errors inside the getter and setter should only trigger if something > really goes wrong now, for example, with a broken migration stream. Or > when we forget to update the storage key allocation with memory hotplug. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/s390x/s390-skeys.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c > index 56a47fe180..53e16f1b9c 100644 > --- a/hw/s390x/s390-skeys.c > +++ b/hw/s390x/s390-skeys.c > @@ -18,6 +18,7 @@ > #include "qapi/qmp/qdict.h" > #include "qemu/error-report.h" > #include "sysemu/memory_mapping.h" > +#include "exec/address-spaces.h" > #include "sysemu/kvm.h" > #include "migration/qemu-file-types.h" > #include "migration/register.h" > @@ -86,6 +87,12 @@ void hmp_info_skeys(Monitor *mon, const QDict *qdict) > return; > } > > + if (!address_space_access_valid(&address_space_memory, > + addr & TARGET_PAGE_MASK, TARGET_PAGE_SIZE, > + false, MEMTXATTRS_UNSPECIFIED)) { > + monitor_printf(mon, "Error: The given address is not valid\n"); I think the code should return here? > + } > + > r = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key); > if (r < 0) { > monitor_printf(mon, "Error: %s\n", strerror(-r)); > @@ -197,11 +204,6 @@ static int qemu_s390_skeys_enabled(S390SKeysState *ss) > return 1; > } > > -/* > - * TODO: for memory hotplug support qemu_s390_skeys_set and qemu_s390_skeys_get > - * will have to make sure that the given gfn belongs to a memory region and not > - * a memory hole. > - */ > static int qemu_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn, > uint64_t count, uint8_t *keys) > { > Thomas
On 06.08.21 10:53, Thomas Huth wrote: > On 05/08/2021 17.28, David Hildenbrand wrote: >> Let's validate the given address and report a proper error in case it's >> not. All call paths now properly check the validity of the given GFN. >> Remove the TODO. >> >> The errors inside the getter and setter should only trigger if something >> really goes wrong now, for example, with a broken migration stream. Or >> when we forget to update the storage key allocation with memory hotplug. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/s390x/s390-skeys.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c >> index 56a47fe180..53e16f1b9c 100644 >> --- a/hw/s390x/s390-skeys.c >> +++ b/hw/s390x/s390-skeys.c >> @@ -18,6 +18,7 @@ >> #include "qapi/qmp/qdict.h" >> #include "qemu/error-report.h" >> #include "sysemu/memory_mapping.h" >> +#include "exec/address-spaces.h" >> #include "sysemu/kvm.h" >> #include "migration/qemu-file-types.h" >> #include "migration/register.h" >> @@ -86,6 +87,12 @@ void hmp_info_skeys(Monitor *mon, const QDict *qdict) >> return; >> } >> >> + if (!address_space_access_valid(&address_space_memory, >> + addr & TARGET_PAGE_MASK, TARGET_PAGE_SIZE, >> + false, MEMTXATTRS_UNSPECIFIED)) { >> + monitor_printf(mon, "Error: The given address is not valid\n"); > > I think the code should return here? Whoops, very right. Thanks!
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c index 56a47fe180..53e16f1b9c 100644 --- a/hw/s390x/s390-skeys.c +++ b/hw/s390x/s390-skeys.c @@ -18,6 +18,7 @@ #include "qapi/qmp/qdict.h" #include "qemu/error-report.h" #include "sysemu/memory_mapping.h" +#include "exec/address-spaces.h" #include "sysemu/kvm.h" #include "migration/qemu-file-types.h" #include "migration/register.h" @@ -86,6 +87,12 @@ void hmp_info_skeys(Monitor *mon, const QDict *qdict) return; } + if (!address_space_access_valid(&address_space_memory, + addr & TARGET_PAGE_MASK, TARGET_PAGE_SIZE, + false, MEMTXATTRS_UNSPECIFIED)) { + monitor_printf(mon, "Error: The given address is not valid\n"); + } + r = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key); if (r < 0) { monitor_printf(mon, "Error: %s\n", strerror(-r)); @@ -197,11 +204,6 @@ static int qemu_s390_skeys_enabled(S390SKeysState *ss) return 1; } -/* - * TODO: for memory hotplug support qemu_s390_skeys_set and qemu_s390_skeys_get - * will have to make sure that the given gfn belongs to a memory region and not - * a memory hole. - */ static int qemu_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn, uint64_t count, uint8_t *keys) {
Let's validate the given address and report a proper error in case it's not. All call paths now properly check the validity of the given GFN. Remove the TODO. The errors inside the getter and setter should only trigger if something really goes wrong now, for example, with a broken migration stream. Or when we forget to update the storage key allocation with memory hotplug. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/s390x/s390-skeys.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)